Conversation
|
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
|
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13033 +/- ##
============================================
+ Coverage 18.01% 18.02% +0.01%
- Complexity 16474 16621 +147
============================================
Files 5976 6024 +48
Lines 537858 542229 +4371
Branches 66049 66458 +409
============================================
+ Hits 96882 97742 +860
- Misses 430052 433464 +3412
- Partials 10924 11023 +99
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds Keycloak as an additional OAuth2/OIDC provider by extending the OAuth provider model/API/schema and exposing new provider URL fields in the UI, along with a new Keycloak authenticator implementation and tests.
Changes:
- Add
authorizeurlandtokenurlfields end-to-end (DB schema, VO/response objects, API commands, and UI config/i18n). - Register a new
KeycloakOAuth2Providerand include it in the default provider order. - Update GitHub token exchange to use a configurable token URL instead of a hardcoded endpoint.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/views/auth/Login.vue | Adds Keycloak social login button and consumes authorizeurl from listOauthProvider. |
| ui/src/config/section/config.js | Extends OAuth provider UI config to edit/display authorizeurl/tokenurl and adds keycloak to provider options. |
| ui/public/locales/en.json | Adds UI labels for Authorize URL and Token URL. |
| ui/public/assets/keycloak.svg | Adds Keycloak icon asset for the login UI. |
| plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2ProviderTest.java | Introduces unit tests for the new Keycloak provider. |
| plugins/user-authenticators/oauth2/src/main/resources/META-INF/cloudstack/oauth2/spring-oauth2-context.xml | Registers Keycloak provider bean and adds it to default ordering. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/vo/OauthProviderVO.java | Adds authorizeUrl and tokenUrl columns/fields to the OAuth provider entity. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java | Implements Keycloak OAuth2/OIDC flow using token endpoint + ID token parsing. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/google/GoogleOAuth2Provider.java | Minor refactor/cleanup and fixes a format string. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/github/GithubOAuth2Provider.java | Switches GitHub token endpoint to DB-configured tokenUrl. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/response/OauthProviderResponse.java | Extends API response to include authorizeurl and tokenurl. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/UpdateOAuthProviderCmd.java | Adds new parameters and returns them in response. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java | Adds new parameters and validates Keycloak requires the URLs. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/ListOAuthProvidersCmd.java | Includes new URL fields in list responses. |
| plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImpl.java | Plumbs new URL fields through register/update persistence. |
| plugins/user-authenticators/oauth2/pom.xml | Adds CXF JOSE dependency used for JWT parsing. |
| engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql | Adds authorize_url and token_url columns to cloud.oauth_provider. |
| api/src/main/java/org/apache/cloudstack/api/ApiConstants.java | Adds API constants for authorizeurl and tokenurl. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private void validateIdToken(String idTokenStr, OauthProviderVO provider) { | ||
| JwsJwtCompactConsumer jwtConsumer = new JwsJwtCompactConsumer(idTokenStr); | ||
| JwtClaims claims = jwtConsumer.getJwtToken().getClaims(); | ||
|
|
||
| if (!claims.getAudiences().contains(provider.getClientId())) { | ||
| throw new CloudAuthenticationException("Audience mismatch"); | ||
| } | ||
| } | ||
|
|
||
| private String obtainEmail(String idTokenStr, OauthProviderVO provider) { | ||
| JwsJwtCompactConsumer jwtConsumer = new JwsJwtCompactConsumer(idTokenStr); | ||
| JwtClaims claims = jwtConsumer.getJwtToken().getClaims(); | ||
|
|
||
| if (!claims.getAudiences().contains(provider.getClientId())) { | ||
| throw new CloudAuthenticationException("Audience mismatch"); | ||
| } | ||
|
|
||
| return (String) claims.getClaim("email"); |
There was a problem hiding this comment.
The ID token is parsed with JwsJwtCompactConsumer but its signature is never verified (and the tests even use {"alg":"none"} tokens). This allows token forgery. Please perform proper OIDC validation: verify JWS signature against the provider's JWKS, and validate standard claims (exp/iat, iss, aud, nonce) before trusting the email claim.
There was a problem hiding this comment.
I agree with this comment. The fact is that the token is never checked in Google / Github. Should we introduce it ?
There was a problem hiding this comment.
@tazouxme , it makes sense but some conflicting considerations:
- How much effort will this be?
- How likely is an exploit and what is the potential damage?
My gut says this is a security feature we might want and not a direct vulnerability (as people can decide not to use a certain provider). Note that my gut has been known to be wrong.
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17513 |
|
thanks for the contribution @tazouxme , looks useful. Please have a look at co-pilot’s comments and the pre-commit and license check failures. |
|
Looks very useful feature. Thanks for Contributing @tazouxme Will test it out, I had previously tested saml authentication with keycloak https://kiranchavala.in/blog/cloudstack-integration-with-keycloak/ |
|
Thanks for your positive feedback. There's only a valid point from Copilot regarding the Token validation. Signature should be validated but currently not done for Google / Github. Should this be introduced ? |
If you feel like implementing it, your efforts are appreciated. I think it can be a separate PR. |
Description
Add Keycloak as a new OAuth provider. This PR adds two new fields in the UI and in the DB:
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Screenshots (if appropriate):
How Has This Been Tested?
Manual testing using local Keycloak instance with basic Realm